Repository navigation
Conversation
The runtime source-map decoder (parse_json in src/sourcemap/lib.rs, used to remap error.stack for // @Bun pragma files) rejected any map whose sourcesContent key was absent or null with InvalidSourceMap, and also rejected maps where sourcesContent.length != sources.length. Per the source-map v3 spec, sourcesContent is optional. Node's --enable-source-maps and most other consumers accept these maps. Treat absent/null sourcesContent as no-content-available and drop the strict length check; the per-index bounds check when extracting content already handles short arrays.
|
Status: reduced to the decode fix in 00b8ba7 (merged with
|
|
No actionable comments were generated in the recent review. WalkthroughThe source-map parser now accepts missing or null ChangesSource map content handling
Possibly related issues
Possibly related PRs
Suggested reviewers: Merge Risk:
|
There was a problem hiding this comment.
LGTM — small, spec-aligned relaxation with good test coverage.
What was reviewed:
parse_jsonnow treats absent/nullsourcesContentasNoneand drops the length-equality check; the per-index bounds guard oncontent_slicestill protects short/empty arrays.Vec::with_capacityswitched tosources_paths.items().len(), which is what the loop actually iterates — correct now that the two lengths are no longer forced equal.- Tests cover absent/null/empty-array plus a
[null]control that already passed on main, usetest.concurrent.eachwith proper pipe draining, and assert the remapped location rather than just absence-of-warning.
Extended reasoning...
Overview
Relaxes the runtime source-map JSON decoder in src/sourcemap/lib.rs to accept maps where sourcesContent is absent, null, or shorter than sources, instead of rejecting the whole map with InvalidSourceMap. Per the source-map v3 spec this field is optional, and Node/other consumers already accept these shapes. Net Rust change is ~20 lines: the field lookup becomes Option<_>, the strict length-equality check is removed, and the single downstream consumer (content_slice) matches on the Option while retaining its existing per-index bounds check. Four new parametrized subprocess tests added to test/js/node/module/sourcemap.test.js.
Security risks
None. This is a read-only decoder used to remap error.stack for // @bun-pragma files. Relaxing acceptance of an optional field does not introduce new indexing or allocation driven by untrusted lengths — the only array index into sourcesContent is still guarded by (idx as usize) < items().len(), and the with_capacity argument now derives from sources (which was already required and iterated). A longer-than-sources array is now also accepted, but extra entries are simply never indexed.
Level of scrutiny
Low-to-moderate. The change is mechanical (required → optional), touches a single function, and only widens acceptance in the direction the spec mandates. Failure mode is bounded: worst case is source_contents: None in the returned ParseUrl, which callers already handle (the pre-existing [null] / "" paths produced the same). The StandaloneModuleGraph serializer path is untouched, as noted in the PR — it consumes bun's own bundler output which always carries content.
Other factors
- Tests follow harness conventions:
test.concurrent.eachfor independent subprocess cases,tempDir/bunEnv/bunExe, concurrent pipe drain, exit-code asserted last. - The
[null]variant is an explicit control that passes on main, giving confidence the test fixture (hand-rolled VLQ segment mapping generated 3:3 → original 5:3) is itself correct. - PR description shows
USE_SYSTEM_BUN=1fails 3/4 new cases andbun bd testpasses all 18, and robobun independently reproduced. - No prior reviewer comments to address.
…ap-optional-sourcescontent
|
Updated 4:56 AM PT - Nov 11th, 2026
❌ @robobun, your commit 66bd2de has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36582That installs a local version of the PR into your bun-36582 --bun |
…uested index `parse_json` returned `InvalidSourceMap` when `sourcesContent` was absent, was not an array, or had a length other than the length of `sources`. The field only supplies the text of a code frame. A position needs `mappings` and `sources` only, and ECMA-426 decodes an absent field and a length mismatch with no error. `parse_json` no longer reads the key before it decodes the mappings. It reads the entry for the source index that the caller asked for, and only when that index has a `sources` entry. A `sources` entry that is null or not a string is a source named `null`. More maps decode now, and they reach two places that use a source name: - `Lookup::get_source_code` joined the directory of the file with the name. A module that has no directory (a `blob:` URL) and an empty name made that join panic with "slice index starts at 1 but ends at 0", also on main for a map with `sourcesContent: [null]`. It now reads no file when the path of the module is not absolute. - An empty source name is the file itself. `get_source_map_impl` gives it the name of the file when the map is decoded. The tests are in the files that already cover the runtime decoder and the printed error.
|
@dylan-conway this PR overlaps #46148. Both rewrite the Two questions:
Three results from the tests of this PR that I expect to apply to #46148 too, because it also accepts a map that has no
The Notes in the description have the steps for the last two. |
…ap-optional-sourcescontent
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether dropping the MappingsOnly gate on the sourcesContent lookup in parse_json changes the error.stack path — it does not, since MappingsOnly yields source_index = None at src/sourcemap/lib.rs:1015, so no content is fetched or copied there.
Extended reasoning...
The change relaxes sourcesContent and sources validation in the Rust source-map parser (src/sourcemap/lib.rs), rewrites empty source names to the module basename, and guards the on-disk source read in src/sourcemap/Mapping.rs for non-absolute base filenames; it parses untrusted map JSON but the new index lookups are bounds-checked via Option. Inline findings are posted on the parse relaxation, so a human should weigh those; this note only records the one additional path examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| let sources_paths = match json | ||
| .get(b"sources") | ||
| .ok_or(crate::Error::InvalidSourceMap)? |
There was a problem hiding this comment.
🔴 Users running a // @ bun bundle that sits next to a stale <file>.map written by another tool (tsc's default sourceMap: true emits no sourcesContent) now get stack frames silently remapped by a map for a different generated file, where the base printed Could not decode sourcemap and kept the real frames. The sidecar lookup at src/sourcemap/lib.rs:517-538 reads <file>.map with no sourceMappingURL or debugId reference, and parse_json at lib.rs:916-924 no longer rejects it. Fix: before accepting a map that does not declare sourcesContent, require that the file references it (a sourceMappingURL/debugId comment, or a matching file field), or land the discovery fix the description names before this one.
Why this was flagged
Trigger: tsc emits dist/index.js and dist/index.js.map (no sourcesContent, tsc's default), then bun build src/index.ts --outdir dist --target bun overwrites dist/index.js with a // @ bun bundle and no --sourcemap. On the first error.stack or printed error, get_source_map_impl with load_hint None finds no inline URL and reads dist/index.js.map from disk at src/sourcemap/lib.rs:517-538 without any reference from the file. On the base branch old lib.rs:910-917 returned InvalidSourceMap for the missing sourcesContent, so lib.rs:545-552 printed Could not decode sourcemap in '<file>': InvalidSourceMap and the frames stayed on dist/index.js. After this diff lib.rs:916-924 accepts the map, SavedSourceMap.rs:321 caches it, and VirtualMachine.rs:6758-6797 rewrites the frame URL and position using tsc's mappings for tsc's own output, which do not describe Bun's bundle, so the user sees wrong src/index.ts lines with no warning. No safeguard checks that the sidecar belongs to the file.
Verification: After the diff, parse_json at lib.rs:916-924 only requires mappings and sources; sourcesContent is looked up lazily at lib.rs:1019-1030 and its absence yields content_slice = None without error, so the stale tsc map parses Ok and is adopted as the file's map (lib.rs:539). Stack frames and the uncaught-error preview are then remapped through a map generated for a different file, with no warning.
There was a problem hiding this comment.
Confirmed. It is the second Downsides bullet of the description. My run: tsc && bun build src/index.ts --outdir dist --target bun leaves tsc's dist/index.js.map beside the bundle. main prints the warning and the true frames (dist/index.js:2:25). This branch prints src/index.ts:2:10 for both frames with no warning. main already does that when the leftover map has sourcesContent (tsc with --inlineSources), so the fault is in how the runtime picks a map for a file, not in parse_json.
The fix is a separate change: read <file>.map only when the trailing comments of the file name a map (//# sourceMappingURL= or //# debugId=). This PR should merge after it, and I leave this thread open until then. I did not add a check here that is keyed on sourcesContent. That check keeps the field in control of whether a map is used, and the general check also fixes the case that is on main today.
There was a problem hiding this comment.
Update. This finding still applies, and the PR no longer tries to remove it in code.
The PR is now only the parse_json lines of #46148: an absent or null sourcesContent decodes for every caller. So the map in this thread is applied with no warning. On two release builds of the same tree, after bun build src/index.ts --outdir dist --target bun and with the default map of tsc as dist/index.js.map: main prints the warning and at boom (dist/index.js:4:9). This branch prints at boom (src/index.ts:2:3). main prints that same frame when the leftover map has sourcesContent.
I built a rule for this (use such a map only when the file has its debug id or names it) and took it out again. It decides which <file>.map belongs to a file. No maintainer has ruled on that, and #46148 decides it the other way on the same lines. The rule is kept on the branch robobun/d05b985c/sourcemap-pairing-guard-wip.
The first Downsides bullet of the description puts the question to the reviewer. I leave this thread open for that ruling.
|
A note on one row of the measurements table: On a release build of main (11c3671) that run does not write the snapshot either. printf '// @bun\nimport { test, expect } from "bun:test";\ntest("t", () => { expect(1 + 1).toMatchInlineSnapshot(); });\n//# sourceMappingURL=a.test.js.map\n' > a.test.js
echo '{"version":3,"sources":["orig.test.ts"],"names":[],"mappings":";;AA0CA"}' > a.test.js.map
CI=false bun test ./a.test.jsThe exit code is 1 and the file is not changed. JSC reports a call in a So for this row the PR changes which error the file gets. The matcher throws I looked at a fix for inline snapshots in |
|
An answer to the question about a leftover
|
…ap-optional-sourcescontent
…ap-optional-sourcescontent
`parse_json` refused a map with no `sourcesContent`, so the stack of a `// @bun` file stayed unmapped and `InvalidSourceMap` was printed. A missing or `null` `sourcesContent` now means that the map has no source text. A value that is not a list, a list of another length than `sources`, and a `sources` entry that is not a string are refused as before. `Lookup::get_source_code` reads no file for a source that has no name. In a module with no directory (a `blob:` URL) that read aborted with `slice index starts at 1 but ends at 0`, and a map with no `sourcesContent` now reaches it. The `parse_json` change is the one in #46148. The earlier changes of this branch (every shape of `sourcesContent`, sources with no name, the basename for an empty source name) are taken out. Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
sourcesContent is absent or null
There was a problem hiding this comment.
Beyond the inline findings, I checked the description's claim that the sources/sourcesContent length check and the non-string sources rejection were dropped: the current diff keeps both (src/sourcemap/lib.rs:926 and :937), so the "longer than sources" and null-named-source shapes from the description's table are still rejected in this revision and are not a concern here; the description and its table describe an earlier version of the change.
I did not build or run the new test rows, so the fail-on-main / pass-here claim for them was not verified in this review.
Extended reasoning...
The latest push narrows the change to accepting an absent or null sourcesContent in parse_json and an empty-name guard in Lookup::get_source_code, plus tests in two existing files. The PR description still presents the broader earlier revision (no length check, null source names), which the current code does not match. No security-sensitive surface is touched; the diff is small, but the sibling display-URL path and the stale-sidecar concern from an earlier round remain open inline threads, so a human look is still needed.
Still open from earlier reviews (1):
- Unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/sourcemap/Mapping.rs— nit, pre-existing: with a module whose URL has no directory and a non-empty relative source name, the code-frame read opens a wrong root path such as/rig.ts. The guard at src/sourcemap/Mapping.rs:387 only covers an empty name; dirname of ablob:URL is "" and join_abs_string_buf_checked assumes an absolute cwd, so it emits "/" plus the name with its first byte dropped. Fix: gate the disk read on bun_paths::is_absolute(base_filename) as display_source_url_if_needed already does at Mapping.rs:328, which also covers the empty name. A map without sourcesContent now reaches this path where the base rejected it.Why this was flagged
Trigger: a module imported from a
blob:URL whose inline map hassources: ["orig.ts"]and nosourcesContent(or[null], which the base already accepted), and an uncaught error so Lookup::get_source_code runs. src/paths/resolve_path.rs:646-653 returns "" for dirname of a string with no separator. src/sourcemap/Mapping.rs:396-398 then calls join_abs_string_buf_checked("", buf, ["orig.ts"]). In _join_abs_string_buf at src/paths/resolve_path.rs:2030-2049 the concatenated "orig.ts" has no leading separator, so leading_len is 1 and normalize runs on temp_buf[1..], giving "/rig.ts". src/sourcemap/Mapping.rs:399 reads that file; if it exists its lines are shown as the code frame for orig.ts, otherwise None. The new empty-name guard at Mapping.rs:387 does not apply because the name is not empty. On the base the no-sourcesContent map was rejected so this path was reached only for[null].Verification: nit, pre-existing (the base already reaches the same route when the map carries
sourcesContent: [null]). Trigger: a module loaded from ablob:URL whose inline map has a non-empty relative source name and no usablesourcesContent. src/sourcemap/Mapping.rs:387-389 only returns early forname.is_empty(). Result:"/rig.ts", which Mapping.rs:399 opens.
`test/aot/runner-suites/programs-3.test.ts` runs `inspect-error.test.js` with every program it starts compiled. A compiled `// @bun` program does not read a source map, so the five new tests fail there like `external map whose original source is unavailable` does.
|
The PR is reduced, and the title and the description now match the head. It has the On the review of 00b8ba7:
|
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 1 finding from earlier reviews is still open above.
Still open from earlier reviews (1):
- Unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
Code review completed
Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.
Still open from earlier reviews (1):
- Unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
Related to #15644 and #32070
Problem
// @bunfile whose source map has nosourcesContentkeeps its stack unmapped and printswarn: Could not decode sourcemap in '<file>': InvalidSourceMap.parse_json(src/sourcemap/lib.rs:909-917) requires asourcesContentlist. ECMA-426 makes it optional, and projects strip it frombun buildmaps (Notes).Fix
nullsourcesContentnow means no source text, as in theparse_jsonchange of Runtime plugins: positions go through the inline source map of whatonLoadreturned #46148.Lookup::get_source_codereads no file for an empty source name. In ablob:module that read aborts (panic: slice index starts at 1 but ends at 0), and these maps now reach it.test/js/bun/sourcemap/negative-vlq-mapping.test.tsandtest/js/bun/util/inspect-error.test.jsfail on main and pass here.Background
// @bunfile (bun buildoutput) as it is. The map serves stacks, andsourcesContentonly code frames.<file>.mapby the file's name alone.<file>.mapbelongs to a file. Both need a maintainer ruling and left this PR.Downsides
sourcesContent) is now applied silently, asmainapplies one that hassourcesContent.line(13 to 16) andBun.cronpaths, as onmainwithsourcesContent: [null].Notes
Reports.
sourcesContentremoved from asourcemap: "linked"map, thenInvalidSourceMap. Its author closed it with no reply.nullfor each source (fix(build): keep a null sourcesContent entry per source; split sessionExpiry property chunks kaeawc/auto-mobile#10856), which released Bun accepts. The same works for anyone today.The shapes. ECMA-426 decodes a missing
sourcesContentwith no error step. Fornullit lets a consumer report an error and continue, and rollup writesnull. These three shapes stay refused withInvalidSourceMap, as on main and in #46148:sourcesContentthat is not a list and notnullsourcessourcesentry that is not a stringMeasurements. "main" and "here" are two release builds of the same tree (main 89452d2). They differ only in
src/sourcemap/lib.rsandsrc/sourcemap/Mapping.rs.<file>.mapcases that remap, of 56 (8 ways a file can link the map, 7 shapes of the map)data:map cases that use the inline map, of 21test/js/node/test/fixtures/test426)err.stack, map withsourcesContent, 1 sourceLookup::get_source_code, code frame read from diskLookup::display_source_url_if_needed)parse_json, bytesLookup::get_source_code, bytessourcesContent: two printed errors, one uncaught errorsourcesContent: the same two programsbun test --coverageof a one-source bundle whose map has nosourcesContentlineof a test in such a filetoMatchInlineSnapshot()that must write its snapshot, in such a fileFailed to update inline snapshot, exit 1Inline snapshot matchers must be called from the test file, exit 1blob:module, empty source name,sourcesContent: [null], printed errorerased_get_source_map. The 101-source row is a hardware counter (perf_event_open), three runs each: 577,690 to 577,700 on main and 577,580 to 577,590 here.valgrind,perf,straceandbloatyare not installed on the machine.sourceMappingURLthat names the map, the same after./, the name of another map, equal debug ids, different debug ids, an id in the file only, an id in the map only) with 7 shapes (a full list, absent,null, shorter, longer, a string, anullsource). Only the absent andnullcolumns change, in all 8 link states.sections),validMappingLargeVLQ(sourcemap: decode a VLQ written with more than 8 digits #47380 decodes it) and the two maps with anullentry insources.sourcesContentcosts one more parse for each printed error, because the code frame read decodes the map again and finds no text.Readers of an accepted map. A map that decodes feeds every reader of a source position. For a map with no
sourcesContenteach of these now gives what main gives for the same map withsourcesContent: [null]:bun test --coveragecounts the lines of the original source under the name of the bundle.CallSite.getFileName()andError.prepareStackTracesee the original path.toMatchInlineSnapshot()with no stored snapshot fails in the matcher. A stored snapshot that matches passes on both builds.Bun.cron("./job.js")from such a bundle resolves the path against the original source directory. With nosrc/directory deployed it throwsFailed to resolve path. I measured this in October on an earlier build of this branch, which decoded these maps the same way.The three questions that this PR leaves to a maintainer.
<file>.mapwith nosourcesContentused when nothing in the file links it? This PR says yes, as Runtime plugins: positions go through the inline source map of whatonLoadreturned #46148 does. Repro:bun build src/index.ts --outdir dist --target bun, then put the map that tsc writes for its owndist/index.jsbeside the bundle. main prints the warning anddist/index.js:4:9. This PR printssrc/index.ts:2:3and no warning. main prints the same wrong frame when that map hassourcesContent.<file>.mapwhose debugId is not the debug id of the file #47628 proposes that.onLoadreturned #46148 does.Other open PRs on these lines.
onLoadreturned #46148 has the sameparse_jsonlines. The PR that lands second drops them and keeps both test blocks.--sourcemap=externalbundle that imports a built file as text, where the inline map of that file has nosourcesContent. main printsat boomA (a.ts:2:9)from the bundle's own map. This PR printsat boomA (worker.ts:2:3)from the map in the string. main has the same fault when the map in the string hassourcesContent.Bun.file()blob module an absolute source URL #46842 changes the next statement ofLookup::get_source_code(the directory of a module that has none). The empty-name return here does not depend on it.<file>.mapwhose debugId is not the debug id of the file #47628 change other lines ofparse_jsonand of the<file>.mapread.What left this PR. Earlier heads of this branch accepted every shape of
sourcesContent, named asourcesentry that is not a stringnull, and showed an empty source name as the file. A later local version used a<file>.mapof a refused shape only when the file linked it by debug id or bysourceMappingURL. The self-review found that both decide questions 1 to 3 in code, against the only maintainer-written change on the same lines (#46148). The link rule is kept on the branchrobobun/d05b985c/sourcemap-pairing-guard-wipfor #47628.Not in this change
sections) and aversionother than 3 returnUnsupportedVersion.charset=utf-8;base64data URLs stayUnsupportedFormat. Runtime plugins: positions go through the inline source map of whatonLoadreturned #46148 has that change.sourceRootis not read by this decoder.sourcesContentand no source on disk, the code frame shows the generated lines. main does that forsourcesContent: [null](error printer: remap frames when the original source is unavailable, and not twice after error.stack #38296).sourcesprints the generated file name with the mapped line and column. sourcemap: pass the real source count to the mappings decoder #47240 refuses such a map.serialize_json_source_map_for_standalone(src/standalone_graph/StandaloneModuleGraph.rs) has the same checks. Its only input is the bundler's own map.Nonereturn for an empty name indisplay_source_url_if_neededshows the file itself, and costs 29 instructions for each remapped frame of every map (1,310 to 1,339). It is not in this PR.orig.tsas/rig.ts). main does this forsourcesContent: [null]. Give a relativeBun.file()blob module an absolute source URL #46842 changes that statement. A gate on the module URL would also stop the read of an absolute source name, which main reads today.--sourcemap=linkedbundle whose.mapis missing warnsUnsupportedFormatand not the missing-sourcemaps note.The AOT list.
test/aot/runner-suites/programs-3.test.tsrunsinspect-error.test.jswith every program compiled. A compiled// @bunprogram reads no source map, sotest/aot/runner-suites.programs.txtnow names the five new tests of that file, beside the test of #38296.Suites run with the debug build.
test/js/bun/sourcemap/371 pass.inspect-error.test.js52 pass.test/js/node/module/sourcemap.test.js14 pass.test/cli/test/coverage.test.ts45 pass.test/bake/dev/sourcemap.test.ts2 pass.test/bake/dev/server-sourcemap.test.ts5 pass.compile-sourcemap-internal.test.ts1 pass.test/regression/issue/22003.test.ts1 pass.cargo clippy -p bun_sourcemapis clean. The two changed test files also pass on the release build: 14 and 52.test/aot/runner-suites/programs-3.test.tspasses on the release build.[human-review] gate passed · iteration 3 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 3
evidence per changed file